Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend bitset functionallity #28

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

falbrechtskirchinger
Copy link
Contributor

I went ahead with the PR anyway. (This is for Return-To-The-Roots/s25client#1602.)

  • Add trait-based opt-in.
  • Replace macro-generated operators with template functions.
  • Add more operator overloads.
  • Add utility functions to set, clear, toggle, and query flags.
  • Add unit tests.

I've introduced a local macro ENUM_UTILS_NODISCARD but think it might be useful to add NODISCARD or RTTR_NODISCARD globally. If so, where should I put it? A new file? compile.h, macros.h, future.h, ...

@Flow86
Copy link
Member

Flow86 commented Jul 24, 2023

RTTR_NODISCARD sounds good. yeah perhaps macros.h?

@falbrechtskirchinger
Copy link
Contributor Author

Done. I've also switched to binary literals in the unit tests, which are easier to read, and renamed the type alias T to Int.

Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at enumUtils.h and found it lacking.

What exactly do you need? The idea was to keep it simple and I feel that the templated approach of global operators might increase compile times more than the previous approach using the types directly. But the opposite might also be the case 🤷

Besides that I do like the functions, especially the set with a bool param sounds useful (i.e. "set or clear")

However we may additionally need a type for a bitset distinguished from the bits-enum, see e.g. the isSet function where this might be relevant. If not it should really behave like regular bitsets, i.e. include the suggested change to isSet

libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
tests/testEnumUtils.cpp Outdated Show resolved Hide resolved
tests/testEnumUtils.cpp Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/macros.h Outdated Show resolved Hide resolved
@falbrechtskirchinger
Copy link
Contributor Author

I looked at enumUtils.h and found it lacking.

What exactly do you need? The idea was to keep it simple and I feel that the templated approach of global operators might increase compile times more than the previous approach using the types directly. But the opposite might also be the case shrug

Template instantiation is the expensive operation to be concerned with. The mere existence isn't worth fretting about.
Shouldn't ultimately cost that much more than the plain functions and they aren't that widely used anyway.

However we may additionally need a type for a bitset distinguished from the bits-enum, see e.g. the isSet function where this might be relevant. If not it should really behave like regular bitsets, i.e. include the suggested change to isSet

I don't know what you mean by "regular bitset". A bitset to me is std::vector<bool>. This type of enum is typically known to me as bit flags or flag enums, etc.

@Flamefire
Copy link
Member

Template instantiation is the expensive operation to be concerned with. The mere existence isn't worth fretting about.
Shouldn't ultimately cost that much more than the plain functions and they aren't that widely used anyway.

Exactly: You added operator& templates to the global namespace which are now instantiated every time such an operation is used even on non-bitset/flag-enabled types. That's what I meant. But that might be rare enough, that it doesn't matter.

I don't know what you mean by "regular bitset".

The pattern: A set of bits. Usually an unsigned integer.

@falbrechtskirchinger
Copy link
Contributor Author

Template instantiation is the expensive operation to be concerned with. The mere existence isn't worth fretting about.
Shouldn't ultimately cost that much more than the plain functions and they aren't that widely used anyway.

Exactly: You added operator& templates to the global namespace which are now instantiated every time such an operation is used even on non-bitset/flag-enabled types. That's what I meant. But that might be rare enough, that it doesn't matter.

You're right, of course. SFINAE isn't magic. grep -Er " & | \| " libs/ shows mostly uses that wouldn't proceed to a template instantiation phase. For example, OR-ing FontStyle uses non-template operators. Other uses involve plain integers or ADL in Boost namespaces.

We could also do:

namespace bitset {
inline namespace operators {
// operators here
}
}

Then using namespace bitset::operators or using namespace bitset to bring them into scope. 🤷‍♂️

I don't know what you mean by "regular bitset".

The pattern: A set of bits. Usually an unsigned integer.

Ah, I've seen that during my search just now.

Add new operator overloads to enhance the functionality of bitsets, and
introduce a type trait that enables enums to opt in to being used as
bitsets.
@Flamefire
Copy link
Member

mostly uses that wouldn't proceed to a template instantiation phase. For example, OR-ing FontStyle uses non-template operators. Other uses involve plain integers or ADL in Boost namespaces.

AFAIK it will still instantiate the templates until SFINAE discards them as it has to consider all possibly matching functions and the compiler can't know that those new operators don't match (better) until it instantiated them (up to the SFINAE part)

Anyway: I believe the impact (if any) is small enough that this is worth it.

So I'd go with the current solution as the namespacing of the operators would make it worse to use in e.g. default parameters.

Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the return type for the modify-assign operators

libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
libs/common/include/s25util/enumUtils.h Outdated Show resolved Hide resolved
@falbrechtskirchinger
Copy link
Contributor Author

Just the return type for the modify-assign operators

They are still correct. :-)

@Flamefire
Copy link
Member

They are still correct. :-)

The change-assign operators should return a reference to the assigned-to object, same as the assign operator, shouldn't they?
Yours return a copy/r-value which is not assignable: https://godbolt.org/z/eocW77orn

auto return types don't deduce references. decltype(auto) would but that is overly verbose, so returning Enum& is easiest.

@falbrechtskirchinger
Copy link
Contributor Author

They are still correct. :-)

The change-assign operators should return a reference to the assigned-to object, same as the assign operator, shouldn't they? Yours return a copy/r-value which is not assignable: https://godbolt.org/z/eocW77orn

auto return types don't deduce references. decltype(auto) would but that is overly verbose, so returning Enum& is easiest.

Thanks for checking! That does make a lot more sense even though clangd keeps insisting it deduces as Enum&. Perhaps an issue with my particular version built from master.

Implement utility functions to set, clear, toggle, and query a bitset
for more concise code.
tests/testEnumUtils.cpp Outdated Show resolved Hide resolved
tests/testEnumUtils.cpp Outdated Show resolved Hide resolved
tests/testEnumUtils.cpp Outdated Show resolved Hide resolved
@Flamefire Flamefire merged commit 5a4148d into Return-To-The-Roots:master Jul 26, 2023
@Flamefire
Copy link
Member

@Flow86 Turns out I missed that this Boost macro was added in Boost 1.71 but we still support(ed) 1.69

However 1.71 is the default for Ubuntu 20.04

If you don't mind I'll update all our usage (CI & CMake) to required Boost 1.71+
Otherwise we'd have to go with RTTR_NODISCARD and the extra header

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants